rivertest: implement a Worker type to simplify testing#753
Conversation
d47c2a3 to
f3226b9
Compare
brandur
left a comment
There was a problem hiding this comment.
Cool. Yep, idea looks good to me. Maybe finish it up and add some niceties like docs on the public-facing functions and maybe an example test to demonstrate usage.
| return nil, err | ||
| } | ||
|
|
||
| // TODO: use insertParamsFromConfigArgsAndOptions instead of reimplementing it here? |
There was a problem hiding this comment.
@brandur looked at this one a bit, and insertParamsFromConfigArgsAndOptions can't really be extracted from the main package because there are a number of dependencies on its own types, including interfaces whose methods return structs in that package. There's no way to avoid circular dependencies.
Options I see:
- Live with the ugliness of exporting this API even though it's not useful or intended for public usage
- Essentially reimplement its logic here to create realistic test jobs
- Instead use
rivershared/testfactoryfor building test jobs (wraptestfactory.Job_Buildto return a*Job[T]instead of*riverdriver.JobInsertFullParams.
There was a problem hiding this comment.
Went for option (3) here
f3226b9 to
ae4ee95
Compare
|
@brandur |
ae4ee95 to
1f0f526
Compare
d6f92fc to
52b9190
Compare
|
|
||
| // WorkOpts allocates a synthetic job with the provided arguments and insert | ||
| // options, then works it. | ||
| func (w *Worker[T, TTx]) WorkOpts(ctx context.Context, tb testing.TB, args T, opts river.InsertOpts) error { |
There was a problem hiding this comment.
I opted to make a separate helper method for working a job with custom insert opts because (1) I expect the vast majority of use cases won't need to care about this and will only care about worker + args, and (2) the much more common use case will benefit from not needing to specify , nil at the end of every call.
In doing so I also opted to make the opts arg just a plain struct instead of a pointer, because it should always be provided when calling the more specialized helper method.
There was a problem hiding this comment.
I opted to make a separate helper method for working a job with custom insert opts because (1) I expect the vast majority of use cases won't need to care about this and will only care about worker + args, and (2) the much more common use case will benefit from not needing to specify
, nilat the end of every call.
Works for me I suppose, but just a note that you could state that exact same rationale as to why Insert should've been broken into Insert + InsertOpts.
There was a problem hiding this comment.
You're right and I had a similar thought. Consolidated to just Work (mirroring client Insert method) + WorkJob for a full job struct.
| // In all cases the underlying [river.Worker] will be called with the synthetic | ||
| // job. The execution environment has a realistic River context with access to | ||
| // all River features, including [river.ClientFromContext] and worker | ||
| // middleware. |
There was a problem hiding this comment.
I realized there's a gap in this statement: river.JobCompleteTx won't be able to succeed if the job isn't actually real / isn't in the database. I see two options for working around this:
- Tell users who run into this that they need to first insert a real job, and then use
WorkJobto test working it. - Embed a value in the work context to indicate to
JobCompleteTxthat it's running within a special test helper and it shouldn't worry about whether the job exists.
There was a problem hiding this comment.
Discussed a bit on Slack. Option (2) seems to be the most plausible, although stubbing out real functionality that'd otherwise hit a DB or do so something else in the real world is always a little icky. Hopefully though this is a special case where river.JobCompleteTx is already tested so well that it's not as necessary to do it here.
There was a problem hiding this comment.
This turned out to be more complicated so we decided to leverage the context value to panic if JobCompleteTx is called within a test worker environment and when the job doesn't exist in the database.
52b9190 to
958c2c2
Compare
| if len(rows) == 0 { | ||
| return nil, rivertype.ErrNotFound | ||
| } |
There was a problem hiding this comment.
Uncovered this bug while thinking about how JobCompleteTx would function if called from the new test helpers. Will add it to the changelog.
8398517 to
be5fdb6
Compare
da869ef to
6ec16c2
Compare
4bccc25 to
12f2aaf
Compare
Make this util more useful with variadic args, functioning more similarly to `valutil.FirstNonZero` by returning the first non-empty value or else `nil`.
12f2aaf to
46ec75e
Compare
The `rivertest.Worker` type makes it much simpler to test River workers with a realistic execution environment without needing to touch the database at all. The type leverages a `river.Config` to allocate an inner `Client` and to enable jobs to be easily created with the normal default values, along with full access to features like middleweare. The execution context also contains all the normal stuff that a real context in River might, such as the client and a timeout if configured.
46ec75e to
1a7e5ac
Compare
| // | ||
| // When relying on features that require a database record (such as JobCompleteTx), | ||
| // the job must be inserted into the database first and then executed with | ||
| // WorkJob. |
There was a problem hiding this comment.
We can probably punt on this, but it'd probably make sense to have a test or something that'd make sure users have a sane way to do this.
JobCompleteTx will probably refuse to complete the job unless it's running, and there might not be that easy of a way to manipulate a job from available to running right now. They could Client.Insert, but then it'd get stuck there.
A couple possibilities:
- We could provide a test helper for inserting jobs in any state similar to our internal factories.
- We could have the test helpers here check to see if a job exists in the DB when running. If so, transition the job to
running.
There was a problem hiding this comment.
Yeah, I just ran into that while writing the test for this! 😆 I think it would make sense as a fast follow so we can refine the API a little more.
This can't succeed if the record isn't in the database, so panic if this situation occurs within a test worker environment.
1a7e5ac to
8b36c26
Compare
Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`.
Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`.
Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`.
Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`.
) * allow setting created_at on insert for clock overrides While the client code was attempting to set `created_at` when the clock was stubbed, the driver code was not actually utilizing this at all. This commit makes it possible to set the `created_at` (similar to `scheduled_at`) using a mocked clock. It also removes the ability to set `finalized_at`, which has no need to be set at insert (other than for tests utilizing `JobInsertFull`). * Let test workers call `JobCompleteTx` + test work transaction variants Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`. * always take tx in rivertest.Worker methods * always insert jobs for testworker * use real executor for rivertest worker, alter APIs Leverage the actual job executor for the `rivertest.Worker` type so that it closely matches real job executions. Reduce the exposed APIs so it only has `Work` and `WorkJob`, both of which require a transaction (but the latter works on existing jobs). Rather than returning only an error, also return a `WorkResult` struct that contains additional metadata. Populate it with the final updated job and the event kind to indicate whether the job completed, errored, snoozed, cancelled, etc. --------- Co-authored-by: Brandur <brandur@brandur.org>
Since the early days of the project, we've talked about potentially building some APIs to make it easier to test River workers in a realistic execution environment. This is an attempt to dramatically simplify this process, partly derived from testing implementations I wrote for River Pro's API backend code.
The
rivertest.Workertype makes it much simpler to test River workers with a realistic execution environment without needing to touch the database at all. The type leverages ariver.Configto allocate an innerClientand to enable jobs to be easily created with the normal default values, along with full access to features like middleweare. The execution context also contains all the normal stuff that a real context in River might, such as the client and a timeout if configured.Here's a simple example of how to use this:
This is missing a lot of test coverage and there are some open TODOs like extracting the
insertParamsFromConfigArgsAndOptionsfrom the mainriverpackage sorivertestcan also use it, but this should hopefully illustrate what I think is a pretty smooth API for easier testing of workers than what it otherwise takes today.Related issue: #605
Currently based on #754.